Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Generate phase" fetching uids and gids according to the build context #26

Merged
merged 9 commits into from
Nov 14, 2023

Conversation

pacostas
Copy link
Contributor

@pacostas pacostas commented Oct 11, 2023

Merge After

Summary

During the generate phase, we build the build and the run image for the build phase and for the runtime. At the moment, the uid and gid of the build and run image are hardcoded inside the /bin/generate

var CNB_USER_ID = 1000
var CNB_GROUP_ID = 1000
A Better approach is to let generate phase propagate on the build and run images, the uid and gid that the cnb user has on the current context that the app is being built. This approach is preferable as it also matches the uid and gids we specify on this PR paketo-community/ubi-base-stack#22

The implementation of this pr is as follows:

  • Instead of having hardcoded the values of uids and gids inside the generate binary, we fetch the uid and gid of the cnb user from the etc/passwd file.

Use Cases

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have added an integration test, if necessary.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@pacostas pacostas requested a review from a team as a code owner October 11, 2023 15:27
@pacostas pacostas force-pushed the propagating-uids-gids branch 2 times, most recently from 1bc3cbe to 38f22db Compare October 13, 2023 13:58
@pacostas pacostas force-pushed the propagating-uids-gids branch 4 times, most recently from f488259 to 4b2d60c Compare October 23, 2023 10:48
generate.go Outdated
return DuringBuildPermissionsGetter{get_etc_passwd_file_content: epfg}
}

func Generate(dependencyManager DependencyManager, logger scribe.Emitter, duringBuildPermissionsGetter DuringBuildPermissionsGetter) packit.GenerateFunc {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it would be simpler/cleaner to

  1. Just pass in the UID and GUID in a structure to Generate. The tests would just hard code those values when testing Generate
  2. Make the function to get the UID and GUID from the password file take a filename. We could then add tests which used a dummy password file(s) in the test directories to validate the functionality of reading extracting the UIDs and GUIDs
  3. Update main.go to simply call the function to get the UID and GUID with /etc/passwd and call Generate with the structure returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed the changes, feel free to have a look and give me more feedback :) @mhdawson @thitch97

@pacostas pacostas force-pushed the propagating-uids-gids branch from 4b2d60c to dd38c12 Compare October 27, 2023 17:55
@thitch97 thitch97 added the semver:patch A change requiring a patch version bump label Oct 27, 2023
generate_test.go Outdated Show resolved Hide resolved
@pacostas pacostas force-pushed the propagating-uids-gids branch from 85c805e to 49110a4 Compare October 27, 2023 18:45
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thitch97 thitch97 merged commit 77547a7 into paketo-community:main Nov 14, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants